Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SDK: update error variants in Feature::from_account_info #33750

Merged
merged 1 commit into from
Oct 20, 2023

Conversation

buffalojoec
Copy link
Contributor

Problem

Feature::from_account_info(..) checks that the owner field matches Feature111111111111111111111111111111111111 but it throws ProgramError::InvalidArgument.

It also throws ProgramError::InvalidArgument on deserialization failure.

Summary of Changes

Update these errors to be more clear.

@codecov
Copy link

codecov bot commented Oct 18, 2023

Codecov Report

Merging #33750 (32d5322) into master (fbded92) will decrease coverage by 0.1%.
Report is 1147 commits behind head on master.
The diff coverage is 88.5%.

@@            Coverage Diff            @@
##           master   #33750     +/-   ##
=========================================
- Coverage    81.8%    81.8%   -0.1%     
=========================================
  Files         766      807     +41     
  Lines      209094   217936   +8842     
=========================================
+ Hits       171138   178324   +7186     
- Misses      37956    39612   +1656     

@CriesofCarrots
Copy link
Contributor

@t-nelson , can you please sanity check us on this change? I don't see any callers of Feature::from_account_info() in the monorepo or SPL.

@CriesofCarrots CriesofCarrots added v1.16 PRs that should be backported to v1.16 v1.17 PRs that should be backported to v1.17 labels Oct 18, 2023
@CriesofCarrots
Copy link
Contributor

CriesofCarrots commented Oct 18, 2023

Another way we could address this is by duplicating/moving the struct Feature definition into spl-feature-gate. But maybe it's better to leave it so that updating the interface is painful.

@t-nelson
Copy link
Contributor

it's not clear to me why this method returns a ProgramError at all. doesn't seem to be used in runtime so making this change w/o a feature gate shouldn't break consensus. *::IncorrectProgramId is very much my least favorite of this legacy error codes. it misleads basically 100% of devs/users upon first encounter

@CriesofCarrots
Copy link
Contributor

it's not clear to me why this method returns a ProgramError at all.

I'm not sure why it was written this way originally, especially as there's no caller, but we're about to start using it in a bpf program.

*::IncorrectProgramId is very much my least favorite of this legacy error codes

What variant do you recommend instead? It seems to be the most correct when an account isn't owned by the expected program.

@buffalojoec
Copy link
Contributor Author

What variant do you recommend instead? It seems to be the most correct when an account isn't owned by the expected program.

*::IllegalOwner?

@t-nelson
Copy link
Contributor

IllegalOwner should really be IllegalWithSeedOwner. that error was added to return when system_instruction::Instruction::CreateWithSeed addresses would collide with PDAs. there's an InstructionError::InvalidAccountOwner that's most appropriate and has been used for failed ownership checks elsewhere. there's no ProgramError analog today, but maybe that's the problem. afaik ProgramError is a somewhat dubious clone of InstructionError. not having InvalidAccountOwner represented there means the TryFrom<InstructionError> impl returns Err, which is probably a bug. probably add that in a separate pr, then use it here

@buffalojoec
Copy link
Contributor Author

there's an InstructionError::InvalidAccountOwner that's most appropriate and has been used for failed ownership checks elsewhere. there's no ProgramError analog today, but maybe that's the problem. afaik ProgramError is a somewhat dubious clone of InstructionError. not having InvalidAccountOwner represented there means the TryFrom<InstructionError> impl returns Err, which is probably a bug. probably add that in a separate pr, then use it here

#33766

@CriesofCarrots CriesofCarrots removed the v1.16 PRs that should be backported to v1.16 label Oct 19, 2023
@buffalojoec
Copy link
Contributor Author

Swapped *::IncorrectProgramId for *::InvalidAccountOwner!

@buffalojoec buffalojoec merged commit 6b1e9b8 into solana-labs:master Oct 20, 2023
41 checks passed
mergify bot pushed a commit that referenced this pull request Oct 20, 2023
buffalojoec added a commit that referenced this pull request Oct 20, 2023
…ckport of #33750) (#33780)

SDK: update error variants in `Feature::from_account_info` (#33750)

(cherry picked from commit 6b1e9b8)

Co-authored-by: Joe C <joe.caulfield@solana.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v1.17 PRs that should be backported to v1.17
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants